Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Team Dashboard): conditional visibility of buttons #361

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

HenerHoop
Copy link

@HenerHoop
Copy link
Author

Copy link

github-actions bot commented Nov 16, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 66.87 KB (+0.1% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 27.77 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 135.58 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 243.24 KB (+0.03% 🔺)

@anoblet
Copy link
Collaborator

anoblet commented Nov 16, 2023

This looks good :)

We should get into the practice of using the nothing sentinel (https://lit.dev/docs/api/templates/#nothing) as opposed to returning an empty string in ternaries. This means lit throws away the template part for future evaluations, a small performance boost.

@HenerHoop HenerHoop force-pushed the hener/fix/team-dashboard-conditional-buttons branch from 2ed15e0 to 33472c9 Compare November 16, 2023 22:28
@HenerHoop
Copy link
Author

This looks good :)

We should get into the practice of using the nothing sentinel (https://lit.dev/docs/api/templates/#nothing) as opposed to returning an empty string in ternaries. This means lit throws away the template part for future evaluations, a small performance boost.

@anoblet Thanks for this information. I updated the code.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HenerHoop please follow commit messages standard we use. Look and git log and see how it's usually done.

- fix(Team Dashboard): conditional visibility of buttons
+ refactor(cxl-ui): conditional visibility of buttons in team components

I'm actually surprised linter let you pass "Team Dashboard" in scope 🤔

@HenerHoop HenerHoop force-pushed the hener/fix/team-dashboard-conditional-buttons branch from 33472c9 to 295c683 Compare November 17, 2023 08:32
@HenerHoop
Copy link
Author

@HenerHoop please follow commit messages standard we use. Look and git log and see how it's usually done.

- fix(Team Dashboard): conditional visibility of buttons
+ refactor(cxl-ui): conditional visibility of buttons in team components

I'm actually surprised linter let you pass "Team Dashboard" in scope 🤔

@pawelkmpt But why is the logic of front (aybolit) and back-end(wpstarter) commits different? In one, you need to refer to a packet, and in the other, to a class/feature?

@pawelkmpt
Copy link

@pawelkmpt But why is the logic of front (aybolit) and back-end(wpstarter) commits different? In one, you need to refer to a packet, and in the other, to a class/feature?

They were developed separately and commit standards were set at different moments in time. Aybolit is also a fork from 3rd party developer and we followed what was defined there.

Worth mentioning, WPS became monorepo in the beginning of this year. Previously all items from packages directory were separate repositories. The best maintained was institute-plugin.

@pawelkmpt pawelkmpt merged commit 9f89e23 into master Nov 17, 2023
5 checks passed
@HenerHoop
Copy link
Author

They were developed separately and commit standards were set at different moments in time. Aybolit is also a fork from 3rd party developer and we followed what was defined there.

Worth mentioning, WPS became monorepo in the beginning of this year. Previously all items from packages directory were separate repositories. The best maintained was institute-plugin.

But could we standardize this logic?

@pawelkmpt
Copy link

But could we standardize this logic?

It ain't easy to standarize everything, but indeed it could be more explicit.

https://app.clickup.com/2317190/v/dc/26pw6-8023/26pw6-7161

@pawelkmpt pawelkmpt deleted the hener/fix/team-dashboard-conditional-buttons branch November 17, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants